crypto: migrate setFipsCrypto to internal/errors #16428

jasnell · 2017-10-24T05:47:45Z

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced

Also only exports the setFipsCrypto and getFipsCrypto methods on process.binding('crypto') if FIPS mode is available.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto (fips)

lpinca · 2017-10-24T12:57:50Z

doc/api/errors.md

+<a id="ERR_CRYPTO_FIPS_FORCED"></a>
+### ERR_CRYPTO_FIPS_FORCED
+
+Used when an attempt to made to enable or disable FIPS mode in the `crypto`


I understand the meaning but as a non native speaker this sounds like alien English to me.
Is something like "Used when trying to enable or disable FIPS mode in the crypto module and the --force-fips command-line argument is used" equivalent?

heh... I think I just mangled that sentence a bit ;-)

lpinca · 2017-10-24T14:08:06Z

doc/api/errors.md

+<a id="ERR_CRYPTO_FIPS_UNAVAILABLE"></a>
+### ERR_CRYPTO_FIPS_UNAVAILABLE
+
+Used when an attempt to made to enable FIPS mode in the `crypto` module using


This is also a bit hard to grok (at least for me). Feel free to update it :)

jasnell · 2017-10-25T04:36:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10947/

jasnell · 2017-10-25T04:37:05Z

ping @nodejs/tsc

joyeecheung · 2017-10-25T09:55:42Z

https://ci.nodejs.org/job/node-test-commit-linux-fips/11974/nodes=ubuntu1404-64/console

not ok 316 parallel/test-crypto-fips
  ---
  duration_ms: 2.721
  severity: fail
  stack: |-
    Spawned child [pid:9049] with cmd 'require("crypto").fips' expect 0 with args '' OPENSSL_CONF=""
    Child #1 [pid:9049] OK.
    Spawned child [pid:9055] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips' OPENSSL_CONF=undefined
    Child #2 [pid:9055] OK.
    Spawned child [pid:9061] with cmd 'require("crypto").fips' expect 1 with args '--force-fips' OPENSSL_CONF=undefined
    Child #3 [pid:9061] OK.
    Spawned child [pid:9067] with cmd 'require("crypto").fips' expect 1 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF=undefined
    Child #4 [pid:9067] OK.
    Spawned child [pid:9073] with cmd 'require("crypto").fips' expect 1 with args '' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf"
    Child #5 [pid:9073] OK.
    Spawned child [pid:9080] with cmd 'require("crypto").fips' expect 1 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf"
    Child #6 [pid:9080] OK.
    Spawned child [pid:9086] with cmd 'require("crypto").fips' expect 0 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf"
    Child #7 [pid:9086] OK.
    Spawned child [pid:9092] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips,--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
    Child #8 [pid:9092] OK.
    Spawned child [pid:9098] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf"
    Child #9 [pid:9098] OK.
    Spawned child [pid:9104] with cmd 'require("crypto").fips' expect 1 with args '--force-fips,--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
    Child #10 [pid:9104] OK.
    Spawned child [pid:9110] with cmd 'require("crypto").fips' expect 1 with args '--force-fips' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf"
    Child #11 [pid:9110] OK.
    Spawned child [pid:9116] with cmd '(require("crypto").fips = true,require("crypto").fips)' expect 1 with args '' OPENSSL_CONF=undefined
    Child #12 [pid:9116] OK.
    Spawned child [pid:9122] with cmd '(require("crypto").fips = true,require("crypto").fips = false,require("crypto").fips)' expect 0 with args '' OPENSSL_CONF=undefined
    Child #13 [pid:9122] OK.
    Spawned child [pid:9128] with cmd '(require("crypto").fips = true,require("crypto").fips)' expect 1 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
    Child #14 [pid:9128] OK.
    Spawned child [pid:9134] with cmd '(require("crypto").fips = false,require("crypto").fips)' expect 0 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF=undefined
    Child #15 [pid:9134] OK.
    Spawned child [pid:9140] with cmd '(require("crypto").fips = false,require("crypto").fips)' expect 0 with args '--enable-fips' OPENSSL_CONF=undefined
    Child #16 [pid:9140] OK.
    Spawned child [pid:9146] with cmd 'require("crypto").fips = false' expect "Error [ERR_CRYPTO_FIPS_UNAVAILABLE]: Cannot set FIPS mode in a non-FIPS build." with args '--force-fips' OPENSSL_CONF=undefined
    assert.js:45
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: false == true
        at responseHandler (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-crypto-fips.js:51:14)
        at testHelper (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-crypto-fips.js:59:3)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-crypto-fips.js:210:1)
        at Module._compile (module.js:596:30)
        at Object.Module._extensions..js (module.js:607:10)
        at Module.load (module.js:515:32)
        at tryModuleLoad (module.js:478:12)
        at Function.Module._load (module.js:470:3)
        at Function.Module.runMain (module.js:637:10)
        at startup (bootstrap_node.js:191:16)
  ...

joyeecheung

Missed some errors in CI

joyeecheung · 2017-10-25T10:04:39Z

test/parallel/test-crypto-fips.js

@@ -10,7 +10,9 @@ const fixtures = require('../common/fixtures');

 const FIPS_ENABLED = 1;
 const FIPS_DISABLED = 0;
-const FIPS_ERROR_STRING = 'Error: Cannot set FIPS mode';
+const FIPS_ERROR_STRING =
+  'Error [ERR_CRYPTO_FIPS_UNAVAILABLE]: Cannot set FIPS mode in a ' +


There should be two flavors of this string, the tests below needs update

jasnell · 2017-10-25T17:16:04Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/10973/

jasnell · 2017-10-25T17:16:14Z

@joyeecheung .... PTAL

lpinca · 2017-10-25T17:27:01Z

lib/crypto.js

@@ -182,7 +182,9 @@ function setFipsDisabled() {
  throw new errors.Error('ERR_CRYPTO_FIPS_UNAVAILABLE');
 }

-function setFipsForced() {
+function setFipsForced(val) {
+  if (val && fipsForced)


Why the check on fipsForced? Isn't setFipsForced only called when fipsForced is true?

heh, good point ;-) this only needs to check if val is truthy :-)

jasnell · 2017-10-25T18:31:49Z

CI is good on fips now!

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced

jasnell · 2017-10-26T21:40:30Z

ping @joyeecheung

mcollina

LGTM

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced PR-URL: #16428 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2017-10-27T15:18:56Z

Landed in ee76f31

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced PR-URL: nodejs/node#16428 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell added crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 24, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 24, 2017

lpinca approved these changes Oct 24, 2017

View reviewed changes

lpinca reviewed Oct 24, 2017

View reviewed changes

targos approved these changes Oct 25, 2017

View reviewed changes

joyeecheung approved these changes Oct 25, 2017

View reviewed changes

joyeecheung requested changes Oct 25, 2017

View reviewed changes

jasnell force-pushed the crypto-fips-internal-errors branch from ae963ed to 1e4c943 Compare October 25, 2017 17:15

lpinca reviewed Oct 25, 2017

View reviewed changes

jasnell force-pushed the crypto-fips-internal-errors branch from 0391080 to 5438770 Compare October 25, 2017 18:39

crypto: migrate setFipsCrypto to internal/errors

a4e0c01

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced

jasnell force-pushed the crypto-fips-internal-errors branch from 5438770 to a4e0c01 Compare October 26, 2017 21:39

joyeecheung approved these changes Oct 27, 2017

View reviewed changes

mcollina approved these changes Oct 27, 2017

View reviewed changes

jasnell closed this Oct 27, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: migrate setFipsCrypto to internal/errors #16428

crypto: migrate setFipsCrypto to internal/errors #16428

jasnell commented Oct 24, 2017

lpinca Oct 24, 2017

jasnell Oct 24, 2017

lpinca Oct 24, 2017

jasnell commented Oct 25, 2017

jasnell commented Oct 25, 2017

joyeecheung commented Oct 25, 2017

joyeecheung left a comment

joyeecheung Oct 25, 2017

jasnell commented Oct 25, 2017

jasnell commented Oct 25, 2017

lpinca Oct 25, 2017 •

edited

Loading

jasnell Oct 25, 2017

jasnell commented Oct 25, 2017

jasnell commented Oct 26, 2017

mcollina left a comment

jasnell commented Oct 27, 2017

crypto: migrate setFipsCrypto to internal/errors #16428

crypto: migrate setFipsCrypto to internal/errors #16428

Conversation

jasnell commented Oct 24, 2017

Checklist

Affected core subsystem(s)

lpinca Oct 24, 2017

Choose a reason for hiding this comment

jasnell Oct 24, 2017

Choose a reason for hiding this comment

lpinca Oct 24, 2017

Choose a reason for hiding this comment

jasnell commented Oct 25, 2017

jasnell commented Oct 25, 2017

joyeecheung commented Oct 25, 2017

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung Oct 25, 2017

Choose a reason for hiding this comment

jasnell commented Oct 25, 2017

jasnell commented Oct 25, 2017

lpinca Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell Oct 25, 2017

Choose a reason for hiding this comment

jasnell commented Oct 25, 2017

jasnell commented Oct 26, 2017

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Oct 27, 2017

lpinca Oct 25, 2017 •

edited

Loading